Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

Improve ES Sink: #1260

Merged
merged 5 commits into from
Sep 2, 2016
Merged

Conversation

AlmogBaku
Copy link
Contributor

@AlmogBaku AlmogBaku commented Aug 16, 2016

  • support AWS authentication for ES servers
  • print errors if exists
  • change elastic library to the proper url(using v.3)
  • fixes ES Sink URI doesn't support single node config(only with ?nodes)
  • uses gopkg namespace for elasticsearch rather than the github(as specified in the library documentation)
  • fixes wrong ID serialization

This change is Reviewable

 - support AWS authentication for ES servers
 - print errors if exists
 - change elastic library to the proper url(using v.3)
 - fixes ES Sink URI doesn't support single node config(only with ?nodes)
 - bring back the go-extpoint that was required by the doc
@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

revert Makefile change [mistakenly commited]
@AlmogBaku
Copy link
Contributor Author

ping @piosz @vishh @mwielgus @mvdan

@piosz
Copy link
Contributor

piosz commented Aug 22, 2016

@huangyuqi could you please take a look?

@AlmogBaku
Copy link
Contributor Author

@Thermi can you help us out here and do a code review also?

@@ -3,7 +3,8 @@
"GoVersion": "go1.6",
"GodepVersion": "v74",
"Packages": [
"./..."
"./...",
"github.com/progrium/go-extpoints"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove progrium. We no longer use this, though the documentation is outdated.

@AlmogBaku
Copy link
Contributor Author

Godeps/Godeps.json, line 7 [r2] (raw file):

Previously, piosz (Piotr Szczesniak) wrote…

Please remove progrium. We no longer use this, though the documentation is outdated.

Done.

Comments from Reviewable

@AlmogBaku
Copy link
Contributor Author

@piosz I've fixed the issue you raised (and removed the go-ext dependency).

@huangyuqi @Thermi did you had a chance to review it?

@huangyuqi
Copy link

@piosz @AlmogBaku yes, I'd be happy to review this PR. Thanks.

@@ -76,7 +78,7 @@ func CreateElasticSearchConfig(uri *url.URL) (*ElasticSearchConfig, error) {
var esConfig ElasticSearchConfig
opts, err := url.ParseQuery(uri.RawQuery)
if err != nil {
return nil, fmt.Errorf("failed to parser url's query string: %s", err)
return nil, fmt.Errorf("fFailed to parser url's query string: %s", err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo error
fFailed ==> Failed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@piosz
Copy link
Contributor

piosz commented Sep 1, 2016

Thanks @huangyuqi for offering your help.

@AlmogBaku
Copy link
Contributor Author

common/elasticsearch/elasticsearch.go, line 81 [r1] (raw file):

Previously, AlmogBaku (Almog Baku) wrote…

done

Done.

Comments from Reviewable

@AlmogBaku
Copy link
Contributor Author

Review status: 0 of 199 files reviewed at latest revision, 7 unresolved discussions.


common/elasticsearch/elasticsearch.go, line 133 [r1] (raw file):

Previously, AlmogBaku (Almog Baku) wrote…

done

Done.

common/elasticsearch/elasticsearch.go, line 142 [r1] (raw file):

Previously, AlmogBaku (Almog Baku) wrote…

done

Done.

events/sinks/elasticsearch/driver.go, line 87 [r1] (raw file):

Previously, huangyuqi (yuqi huang) wrote…

Yes, i agree, warning is more precise. thanks

Done.

Godeps/Godeps.json, line 7 [r2] (raw file):

Previously, AlmogBaku (Almog Baku) wrote…

Done.

@pio

metrics/sinks/elasticsearch/driver.go, line 102 [r1] (raw file):

Previously, AlmogBaku (Almog Baku) wrote…

I changed it to warning.. It make more sense to me. What do you think?

Done.

Comments from Reviewable

@piosz
Copy link
Contributor

piosz commented Sep 2, 2016

@AlmogBaku thanks for the contribution.
@huangyuqi thanks for the review.

@piosz piosz added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2016
@piosz piosz merged commit 2a7fbcf into kubernetes-retired:master Sep 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants